roles/elasticsearch/tasks/restart_and_verify_elasticsearch_rolling.yml (1)

34-39: Redundant when condition.

The when: _elasticsearch_restart_hosts | length > 0 check on line 39 is unnecessary—looping over an empty list naturally results in zero iterations. Removing it simplifies the code.

Optional simplification

 - name: restart_and_verify_elasticsearch_rolling | Restart requested nodes one at a time
   ansible.builtin.include_tasks: restart_and_verify_elasticsearch_rolling_node.yml
   loop: "{{ _elasticsearch_restart_hosts }}"
   loop_control:
     loop_var: _elasticsearch_restart_host
-  when: _elasticsearch_restart_hosts | length > 0

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@roles/elasticsearch/tasks/restart_and_verify_elasticsearch_rolling.yml`
around lines 34 - 39, The task "restart_and_verify_elasticsearch_rolling |
Restart requested nodes one at a time" redundantly uses "when:
_elasticsearch_restart_hosts | length > 0" before looping; remove that when
condition and rely on looping over "loop: {{ _elasticsearch_restart_hosts }}"
(with loop_control.loop_var: _elasticsearch_restart_host) so the include_tasks
call to restart_and_verify_elasticsearch_rolling_node.yml naturally no-ops for
an empty list.

.gitignore (1)

10-18: Redundant gitignore rule on line 10.

Line 10 (tests/integration/) ignores the directory itself, but line 11 immediately negates it with !tests/integration/. Since line 12 (tests/integration/*) already ignores the directory contents, line 10 is redundant and creates confusion.

Consider removing line 10 if you only want to ignore contents while tracking specific files:

Proposed simplification

-tests/integration/
-!tests/integration/
 tests/integration/*
 !tests/integration/elasticsearch_cluster_settings_contract.yml
 !tests/integration/elasticsearch_template_contract.yml

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.gitignore around lines 10 - 18, Remove the redundant ignore rule
"tests/integration/"; keep "tests/integration/*" and the explicit negations
(e.g., "!tests/integration/elasticsearch_cluster_settings_contract.yml", etc.)
so the directory is not ignored but its contents are selectively
ignored/tracked—delete the lone "tests/integration/" entry from the .gitignore.

tests/fakes/fake_es_rolling_api.py (1)

113-128: Consider adding graceful shutdown capability.

The server blocks indefinitely with threading.Event().wait() which works for the test scenario (killed via PID), but doesn't support graceful shutdown. This is acceptable for test infrastructure, but a signal handler could make debugging easier.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/fakes/fake_es_rolling_api.py` around lines 113 - 128, The main()
function currently blocks forever with threading.Event().wait(), preventing
graceful shutdown; modify main to create a shared threading.Event (e.g.,
stop_event), pass it into serve (or make it accessible to State/serve), and
install signal handlers for SIGINT/SIGTERM that set stop_event so threads can
exit cleanly; after starting threads, wait on stop_event and then join threads
(or allow serve to exit when stop_event.is_set()), ensuring State/serve respect
the stop_event for orderly teardown.

molecule/elasticsearch_default/side_effect.yml (1)

23-26: Add error handling for PID capture.

If Elasticsearch isn't running when this task executes, pgrep returns non-zero and the task fails. Consider adding failed_when: false or checking the return code, especially since this runs at the start of the side effect test.

Proposed fix

     - name: Record ES PID before config change
       ansible.builtin.command: pgrep -f 'org.elasticsearch.bootstrap.Elasticsearch'
       register: _es_pid_before_config_restart
       changed_when: false
+      failed_when: false

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@molecule/elasticsearch_default/side_effect.yml` around lines 23 - 26, The
"Record ES PID before config change" task using the command pgrep -f
'org.elasticsearch.bootstrap.Elasticsearch' can fail if Elasticsearch isn't
running; update this task (the one that registers _es_pid_before_config_restart)
to avoid failing the play by adding explicit error handling such as failed_when:
false (or ignore_errors: true) and keep changed_when: false so the play
continues when pgrep returns non-zero; ensure the task still registers
_es_pid_before_config_restart and downstream tasks check its.rc or stdout to
detect absence of a PID.

tests/integration/rolling_restart_contract.yml (1)

88-94: Assertion patterns may not match JSONL key order.

The assertions at lines 91-92 search for '"path": "/_flush", "port": 19200'. However, looking at the fake API's log() method (context snippet 2), keys are sorted alphabetically (sort_keys=True), so the actual order is: body, method, path, port. The pattern should work since both "path" and "port" appear consecutively in sorted order.

Lines 93-94 assert "primaries" and "null" appear in the log—these likely come from the allocation settings PUT body. Consider adding comments explaining what these assertions verify.

Clarify assertion intent

     - name: Assert both fake nodes were processed with flush
       ansible.builtin.assert:
         that:
           - '_fake_api_log.stdout is search(''"path": "/_flush", "port": 19200'')'
           - '_fake_api_log.stdout is search(''"path": "/_flush", "port": 19201'')'
+          # Verify allocation settings were applied (primaries allocation, null to reset)
           - "_fake_api_log.stdout is search('primaries')"
           - "_fake_api_log.stdout is search('null')"

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/integration/rolling_restart_contract.yml` around lines 88 - 94, The
current assertions in tests/integration/rolling_restart_contract.yml check
literal key order in _fake_api_log.stdout (e.g., '"path": "/_flush", "port":
19200') which is fragile given the fake API's log() uses sort_keys=True; update
the test to either use order-independent checks (e.g., separate contains checks
for '"path": "/_flush"' and '"port": 19200' or a regex that allows any ordering)
or keep the existing checks but add a clear comment referencing the fake API's
log() sort_keys=True behavior and that the assertion expects alphabetic ordering
(and also clarify what the '"primaries"' and '"null"' assertions validate) so
intent is explicit; target the assertions on _fake_api_log and mention
log()/sort_keys=True in the comment or refactor the two lines that combine path
and port into independent assertions.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

roles/elasticsearch/tasks/restart_and_verify_elasticsearch_rolling_node.yml (1)

16-16: ⚠️ Potential issue | 🟠 Major

Require green after allocation is re-enabled.

_elasticsearch_health_statuses is still reused for the post-restart gates, so a run with elasticsearch_config_restart_wait_status: yellow can proceed while replicas are still unassigned. That breaks the rolling-restart contract from issue #121 for the post-reenable phase.

Suggested fix

-    _elasticsearch_health_statuses: "{{ ['green'] if elasticsearch_config_restart_wait_status == 'green' else ['green', 'yellow'] }}"
+    _elasticsearch_pre_restart_health_statuses: "{{ ['green'] if elasticsearch_config_restart_wait_status == 'green' else ['green', 'yellow'] }}"
+    _elasticsearch_post_restart_health_statuses: ['green']
@@
-        - (_elasticsearch_pre_restart_health.json.status | default('')) in _elasticsearch_health_statuses
+        - (_elasticsearch_pre_restart_health.json.status | default('')) in _elasticsearch_pre_restart_health_statuses
@@
-        - (_elasticsearch_post_restart_health.json.status | default('')) in _elasticsearch_health_statuses
+        - (_elasticsearch_post_restart_health.json.status | default('')) in _elasticsearch_post_restart_health_statuses
@@
-        - (_elasticsearch_post_restart_health.json.status | default('')) in _elasticsearch_health_statuses
+        - (_elasticsearch_post_restart_health.json.status | default('')) in _elasticsearch_post_restart_health_statuses

Also applies to: 181-193, 258-270

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@roles/elasticsearch/tasks/restart_and_verify_elasticsearch_rolling_node.yml`
at line 16, The current variable _elasticsearch_health_statuses uses
elasticsearch_config_restart_wait_status for both pre-restart and post-reenable
checks, letting a run proceed with yellow even after shard allocation is
re-enabled; change the post-reenable gate to require ['green'] unconditionally.
Locate uses of _elasticsearch_health_statuses around the post-restart
verification (references to _elasticsearch_health_statuses and
elasticsearch_config_restart_wait_status) and either introduce a separate
variable (e.g., _elasticsearch_health_statuses_post_reenable = ['green']) or
override the value for the post-reenable checks so that after allocation is
re-enabled the code always waits for ['green'].

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

roles/elasticsearch/tasks/elasticsearch-cluster-settings.yml (1)

35-39: ⚠️ Potential issue | 🟠 Major

Normalize booleans before comparison to preserve idempotency.

Line 38 still compares raw string forms; YAML booleans (True/False) can differ from Elasticsearch-returned lowercase values (true/false), causing repeated PUTs.

Suggested fix

         _needs_update: >-
           {% set ns = namespace(changed=false) %}
           {% for key, value in _elasticsearch_effective_cluster_settings.items() %}
-          {%   if _current.get(key) is none or _current[key] | string != value | string %}
+          {%   if value is boolean %}
+          {%     set expected = 'true' if value else 'false' %}
+          {%   else %}
+          {%     set expected = value | string %}
+          {%   endif %}
+          {%   if _current.get(key) is none or (_current[key] | string) != expected %}
           {%     set ns.changed = true %}
           {%   endif %}
           {% endfor %}
           {{ ns.changed }}

As per coding guidelines, "Review for Ansible best practices: idempotency (command/shell needs creates/removes guards), correct handler notifications, proper use of become/become_user, and platform conditionals (Debian vs RHEL)."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@roles/elasticsearch/tasks/elasticsearch-cluster-settings.yml` around lines 35
- 39, The comparison inside _needs_update can misdetect changes because YAML
booleans may be True/False while Elasticsearch returns lowercase true/false;
update the loop comparing _elasticsearch_effective_cluster_settings and _current
so both sides are normalized before comparing: for each key use a normalized
form like (value is boolean ? (value | string | lower) : (value | string)) and
the same normalization for _current[key] (check _current.get(key) is defined
first), then set ns.changed = true only if the normalized strings differ; this
targets the existing symbols _needs_update,
_elasticsearch_effective_cluster_settings, _current and ns.changed.

roles/elasticsearch/handlers/main.yml (1)

20-22: ⚠️ Potential issue | 🟠 Major

Default the group-name variable before indexing groups[...].

These conditions still index groups[elasticstack_elasticsearch_group_name] directly. If this role is used standalone and that variable is unset, the expression can fail before the trailing | default([]) is applied, so the direct/rolling split crashes instead of falling back safely.

Suggested hardening

-      or groups[elasticstack_elasticsearch_group_name] | default([]) | length <= 1
+      or groups[elasticstack_elasticsearch_group_name | default('elasticsearch')] | default([]) | length <= 1
...
-      or groups[elasticstack_elasticsearch_group_name] | default([]) | length <= 1
+      or groups[elasticstack_elasticsearch_group_name | default('elasticsearch')] | default([]) | length <= 1
...
-    - groups[elasticstack_elasticsearch_group_name] | default([]) | length > 1
+    - groups[elasticstack_elasticsearch_group_name | default('elasticsearch')] | default([]) | length > 1

Also applies to: 34-36, 48-49

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@roles/elasticsearch/handlers/main.yml` around lines 20 - 22, The condition
indexes groups[elasticstack_elasticsearch_group_name] directly which can raise
if elasticstack_elasticsearch_group_name is undefined; update all such
expressions (the conditional using elasticsearch_config_restart_strategy and the
other occurrences noted at 34-36 and 48-49) to safely default before indexing,
e.g. use groups.get(elasticstack_elasticsearch_group_name, []) | default([]) or
groups.get(elasticstack_elasticsearch_group_name, []) | length where appropriate
so the expression never attempts to index with an undefined variable; apply this
change to every occurrence of groups[elasticstack_elasticsearch_group_name] in
the handler file.

roles/elasticsearch/tasks/restart_and_verify_elasticsearch_rolling_node.yml (1)

16-16: ⚠️ Potential issue | 🟠 Major

Require green after re-enabling allocation.

This status list is reused by the post-restart health checks, so elasticsearch_config_restart_wait_status: yellow still allows the workflow to advance while the cluster is yellow. Issue #121 requires the post-allocation gate to wait for green before moving to the next node.

Suggested fix

-    _elasticsearch_health_statuses: "{{ ['green'] if elasticsearch_config_restart_wait_status == 'green' else ['green', 'yellow'] }}"
+    _elasticsearch_pre_restart_health_statuses: "{{ ['green'] if elasticsearch_config_restart_wait_status == 'green' else ['green', 'yellow'] }}"
+    _elasticsearch_post_restart_health_statuses: ['green']

Then use _elasticsearch_pre_restart_health_statuses for the pre-restart wait, and _elasticsearch_post_restart_health_statuses for the post-reenable waits.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@roles/elasticsearch/tasks/restart_and_verify_elasticsearch_rolling_node.yml`
at line 16, The current single variable _elasticsearch_health_statuses allows a
yellow cluster to pass post-restart checks; split it into two variables and
update usages: create _elasticsearch_pre_restart_health_statuses = "{{ ['green']
if elasticsearch_config_restart_wait_status == 'green' else ['green','yellow']
}}" and _elasticsearch_post_restart_health_statuses = "['green']" and replace
references that run before the restart with
_elasticsearch_pre_restart_health_statuses and references that run after
re-enabling allocation with _elasticsearch_post_restart_health_statuses so the
post-allocation gate always requires green.